Data: Support object as second argument #235
Conversation
jQuery.data supports passing an object as a second argument and sets data for each entry in that object. This warns the user about each violating non-camelCase key in the object. This also adds a type check to avoid a current bug where jQuery.camelCase throws an error because a non-string is passed to it.
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
src/data.js
Outdated
| jQuery.data = function( elem, name, value ) { | ||
| var curData; | ||
|
|
||
| //Name can be an object, and each entry in the object is meant to be set as data |
| } | ||
| } | ||
|
|
||
| oldData.call( this, elem, sameKeys ); |
There was a problem hiding this comment.
Why not oldData.apply( this, arguments ) similarly to how what the other configuration is handled? You wouldn't have to record sameKeys at all then.
There was a problem hiding this comment.
sameKeys is not the same as the second argument, as it only contains keys that are in camelCase and thus does not need to be handled here.
The other oldData.apply is actually doing the same if it determines the key is already camel case.
There was a problem hiding this comment.
Ah, right, I was comparing it to the second part but there's an early return there if the non-camelCased version of the key was found. It's fine then. 👍
test/data.js
Outdated
| } ); | ||
| } ); | ||
|
|
||
| div = document.createElement( "div" ); |
There was a problem hiding this comment.
Could you move all this code to a separate test? An example name: "jQuery.data() camelCased names (mass setter)".
|
@wjmao88 Sorry for keeping you wait this long with the review. Will you have time to address my comments? If not, I can finish it for you. |
|
This PR should fix issue #198. |
|
@mgol Yes, I'll be able to find time within a day or so. |
|
|
||
| // If the name is transformed, look for the un-transformed name in the data object | ||
| if ( name && name !== jQuery.camelCase( name ) ) { | ||
| if ( name && typeof name === "string" && name !== jQuery.camelCase( name ) ) { |
There was a problem hiding this comment.
I don't think this change is necessary. We only support documented scenarios, while we want to add support for the object as the second parameter here at this point we know it's not an object so we're fine.
There was a problem hiding this comment.
I agree with your reason, which is why I think it should goes straight to jQuery's own .data method, and will throw the same error as when jquery-migrate is not used.
| } | ||
| } | ||
|
|
||
| oldData.call( this, elem, sameKeys ); |
There was a problem hiding this comment.
Ah, right, I was comparing it to the second part but there's an early return there if the non-camelCased version of the key was found. It's fine then. 👍
|
Yes, his reasoning sounds reasonable!
|
|
@wjmao88 Landed, thanks! |
jQuery.data supports passing an object as a second argument and sets data for each entry in that object.
This warns the user about each violating non-camelCase key in the object.
This also adds a type check to avoid a current bug where jQuery.camelCase throws an error because a non-string is passed to it.